-
Notifications
You must be signed in to change notification settings - Fork 290
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve publication onboarding state synchronization. #9469
base: develop
Are you sure you want to change the base?
Conversation
Build files for d17f5bd are ready:
|
Size Change: +178 B (+0.01%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brilliant work on this, thank you @ankitrox !
- I have left a number of comments in the PR. Could you please go through my comments and update the PR accordingly?
- The major issue that I've noticed is that the "publication approved overlay notification" does not appear when the cron event runs and the publication onboarding state changes to complete. Could you investigate?
- I see that the
publicationOnboardingStateLastSyncedAtMs
setting is still used across the codebase, mostly in tests and stories. Can they be removed?
Please let me know if you have any questions. Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should no longer set publicationOnboardingStateLastSyncedAtMs
in the selectPublication
action.
/** | ||
* User options instance. | ||
* | ||
* @since n.e.x.t | ||
* | ||
* @var User_Options | ||
*/ | ||
protected $user_options; | ||
|
||
/** | ||
* Constructor. | ||
* | ||
* @since n.e.x.t | ||
* | ||
* @param Context $context Plugin context. | ||
* @param Options $options Optional. Option API instance. Default is a new instance. | ||
* @param User_Options $user_options Optional. User Option API instance. Default is a new instance. | ||
* @param Authentication $authentication Optional. Authentication instance. Default is a new instance. | ||
* @param Assets $assets Optional. Assets API instance. Default is a new instance. | ||
*/ | ||
public function __construct( | ||
Context $context, | ||
Options $options = null, | ||
User_Options $user_options = null, | ||
Authentication $authentication = null, | ||
Assets $assets = null | ||
) { | ||
parent::__construct( $context, $options, $user_options, $authentication, $assets ); | ||
$this->user_options = $user_options; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not necessary to re-declare the $user_options
class property because the parent class, Module
, already declares the $user_options
class.
Please take a look at other module classes, such as Analytics_4
, on how they use the $user_options
property.
/** | |
* User options instance. | |
* | |
* @since n.e.x.t | |
* | |
* @var User_Options | |
*/ | |
protected $user_options; | |
/** | |
* Constructor. | |
* | |
* @since n.e.x.t | |
* | |
* @param Context $context Plugin context. | |
* @param Options $options Optional. Option API instance. Default is a new instance. | |
* @param User_Options $user_options Optional. User Option API instance. Default is a new instance. | |
* @param Authentication $authentication Optional. Authentication instance. Default is a new instance. | |
* @param Assets $assets Optional. Assets API instance. Default is a new instance. | |
*/ | |
public function __construct( | |
Context $context, | |
Options $options = null, | |
User_Options $user_options = null, | |
Authentication $authentication = null, | |
Assets $assets = null | |
) { | |
parent::__construct( $context, $options, $user_options, $authentication, $assets ); | |
$this->user_options = $user_options; | |
} |
@@ -38,6 +42,8 @@ | |||
use Google\Site_Kit\Modules\Reader_Revenue_Manager\Web_Tag; | |||
use Google\Site_Kit\Modules\Search_Console\Settings as Search_Console_Settings; | |||
use Google\Site_Kit_Dependencies\Google\Service\SubscribewithGoogle as Google_Service_SubscribewithGoogle; | |||
use Google\Site_Kit\Modules\Reader_Revenue_Manager\Synchronize_OnboardingState; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we're at it, let's re-order this import alphabetically. Also, if we do remove the constructor as described above, we'll also need to remove a few unused imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two points:
- Sanitization should be added for the
publicationOnboardingStateChanged
setting. - Existing sanitization should be removed for the legacy
publicationOnboardingStateLastSyncedAtMs
setting.
/** | ||
* Class Google\Site_Kit\Modules\Analytics_4\Synchronize_OnboardingState. | ||
* | ||
* @since n.e.x.t | ||
* @package Google\Site_Kit\Modules\Reader_Revenue_Manager | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The header comment for this file appears to be quite different than other classes.
/** | |
* Class Google\Site_Kit\Modules\Analytics_4\Synchronize_OnboardingState. | |
* | |
* @since n.e.x.t | |
* @package Google\Site_Kit\Modules\Reader_Revenue_Manager | |
*/ | |
/** | |
* Class Google\Site_Kit\Modules\Reader_Revenue_Manager\Synchronize_OnboardingState | |
* | |
* @package Google\Site_Kit\Modules\Reader_Revenue_Manager | |
* @copyright 2024 Google LLC | |
* @license https://www.apache.org/licenses/LICENSE-2.0 Apache License 2.0 | |
* @link https://sitekit.withgoogle.com | |
*/ |
@@ -293,7 +295,6 @@ public function test_get_debug_fields() { | |||
array( | |||
'reader_revenue_manager_publication_id', | |||
'reader_revenue_manager_publication_onboarding_state', | |||
'reader_revenue_manager_publication_onboarding_state_last_synced_at', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to my observation, I don't think any other change apart from this one is needed in this test suite.
$this->assertFalse( $settings['publicationOnboardingStateChanged'] ); | ||
|
||
do_action( Synchronize_OnboardingState::CRON_SYNCHRONIZE_ONBOARDING_STATE ); | ||
$test_synced_at = time(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$test_synced_at = time(); |
// Get cron list and check if the cron is scheduled. | ||
$crons = _get_cron_array(); | ||
$cron_exists = false; | ||
|
||
foreach ( $crons as $timestamp => $cron_events ) { | ||
foreach ( $cron_events as $event_key => $cron_event ) { | ||
if ( Synchronize_OnboardingState::CRON_SYNCHRONIZE_ONBOARDING_STATE === $event_key ) { | ||
$cron_exists = true; | ||
break 2; | ||
} | ||
} | ||
} | ||
|
||
$this->assertTrue( $cron_exists ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we can simplify this.
// Get cron list and check if the cron is scheduled. | |
$crons = _get_cron_array(); | |
$cron_exists = false; | |
foreach ( $crons as $timestamp => $cron_events ) { | |
foreach ( $cron_events as $event_key => $cron_event ) { | |
if ( Synchronize_OnboardingState::CRON_SYNCHRONIZE_ONBOARDING_STATE === $event_key ) { | |
$cron_exists = true; | |
break 2; | |
} | |
} | |
} | |
$this->assertTrue( $cron_exists ); | |
$this->assertTrue( | |
(bool) wp_next_scheduled( Synchronize_OnboardingState::CRON_SYNCHRONIZE_ONBOARDING_STATE ) | |
); |
@@ -170,7 +177,7 @@ export default { | |||
.dispatch( MODULES_ANALYTICS_4 ) | |||
.receiveGetAccountSummaries( { | |||
accountSummaries, | |||
nextPageToken: null, | |||
nextPageToken: '', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to change this.
nextPageToken: '', | |
nextPageToken: null, |
const shouldShowNotification = | ||
isDismissed === false && | ||
showApprovedNotificationUI === true && | ||
! isViewOnly && | ||
dashboardType === VIEW_CONTEXT_MAIN_DASHBOARD; | ||
dashboardType === VIEW_CONTEXT_MAIN_DASHBOARD && | ||
initialPublicationOnboardingStateChanged.current === true && | ||
publicationOnboardingState === ONBOARDING_COMPLETE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, the overlay notification no longer appears for me, because no one sets the value for UI_KEY_READER_REVENUE_MANAGER_SHOW_PUBLICATION_APPROVED_NOTIFICATION
in the CORE_UI
store, which is only done when the sync is run on the client-side.
The logic here should be changed so that the notification is displayed when either the UI store value is set, OR, the new conditions are met.
Somewhat like the following:
const shouldShowNotification =
isDismissed === false &&
! isViewOnly &&
dashboardType === VIEW_CONTEXT_MAIN_DASHBOARD &&
( showApprovedNotificationUI === true ||
( initialPublicationOnboardingStateChanged.current === true &&
publicationOnboardingState === ONBOARDING_COMPLETE ) );
Could you please investigate and ensure that the notification appears?
Thank you @nfmohit for reviewing the PR and adding the feedback. I've addressed the feedback and left response for couple of comments. Over to you for another round of review. Thanks again. |
Summary
Addresses issue:
Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist